fix: cross-route client navigation hangs in Firefox (#652)#690
fix: cross-route client navigation hangs in Firefox (#652)#690Divkix wants to merge 5 commits intocloudflare:mainfrom
Conversation
Replace flushSync-based RSC tree rendering with a two-phase navigation commit that uses startTransition for same-route navigations and synchronous updates for cross-route navigations. This fixes the Firefox hang where startTransition never commits when the entire component tree is replaced, and also resolves the Suspense double-flash from cloudflare#639. Key changes: - navigation.ts: ClientNavigationState on Symbol.for global for module instance safety, render snapshot context for hook consistency during transitions, RSC response snapshot/restore for visited cache, unified navigateClientSide() entry point, history suppression helpers - app-browser-entry.ts: BrowserRoot component with useState-managed tree, NavigationCommitSignal that defers URL commit to useLayoutEffect, visited response cache with LRU eviction, navigation ID counter for stale navigation bailout, manual scroll restoration - link.tsx: remove duplicated helpers, delegate to navigateClientSide - form.tsx: delegate App Router GET navigation to navigateClientSide Closes cloudflare#652
commit: |
There was a problem hiding this comment.
Pull request overview
Fixes a Firefox-specific App Router client-navigation hang by introducing a two-phase navigation commit strategy that uses startTransition only for same-route updates and synchronous updates for cross-route navigations, while deferring URL/history publication until after the new tree commits.
Changes:
- Reworked App Router client navigation flow (snapshot context for hooks, two-phase URL commit, visited response replay cache, and navigation staleness bailout).
- Unified
<Link>and<Form>App Router navigations throughnavigateClientSide()and updated prefetch behavior to snapshot RSC responses for replay. - Added/updated unit + Playwright regression coverage and new fixture routes to validate navigation behavior and hook consistency.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/form.test.ts | Updates window stubs and assertions for the new __VINEXT_RSC_NAVIGATE__ signature and navigation flow. |
| tests/fixtures/app-basic/app/page.tsx | Adds entry links to new navigation regression fixture routes. |
| tests/fixtures/app-basic/app/nav-flash/query-sync/page.tsx | New fixture page for same-route (search param) navigation + Suspense behavior. |
| tests/fixtures/app-basic/app/nav-flash/query-sync/FilterControls.tsx | Client controls validating usePathname/useSearchParams sync during transitions. |
| tests/fixtures/app-basic/app/nav-flash/provider/[id]/page.tsx | New dynamic-param fixture route for cross-route navigation coverage. |
| tests/fixtures/app-basic/app/nav-flash/param-sync/[filter]/page.tsx | New dynamic segment fixture for param-change navigation behavior. |
| tests/fixtures/app-basic/app/nav-flash/param-sync/[filter]/FilterControls.tsx | Client controls validating useParams/usePathname updates on param changes. |
| tests/fixtures/app-basic/app/nav-flash/list/page.tsx | New cross-route destination page used by regression tests. |
| tests/fixtures/app-basic/app/nav-flash/link-sync/page.tsx | New fixture page for link-driven same-route navigation with Suspense. |
| tests/fixtures/app-basic/app/nav-flash/link-sync/FilterLinks.tsx | Client links validating hook snapshot consistency across navigation. |
| tests/e2e/app-router/navigation-regressions.spec.ts | New Playwright regression suite for #652 and related hook/scroll/state behaviors. |
| packages/vinext/src/shims/navigation.ts | Adds navigation global state, render snapshot context, two-phase navigation entry, and prefetch snapshot/consume helpers. |
| packages/vinext/src/shims/link.tsx | Removes duplicated hash/scroll/nav logic and delegates App Router navigation + prefetch to shared helpers. |
| packages/vinext/src/shims/form.tsx | Delegates App Router GET form navigation to navigateClientSide() to align history publishing with commits. |
| packages/vinext/src/server/app-browser-entry.ts | Introduces BrowserRoot state tree management, commit signaling, visited response replay cache, and same-route detection for transition mode. |
| packages/vinext/src/global.d.ts | Extends global typings for updated navigation signature and new prefetch cache entry shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function prefetchRscResponse(rscUrl: string, fetchPromise: Promise<Response>): void { | ||
| const cache = getPrefetchCache(); | ||
| const prefetched = getPrefetchedUrls(); | ||
| const now = Date.now(); | ||
|
|
||
| const entry: PrefetchCacheEntry = { timestamp: now }; | ||
|
|
||
| entry.pending = fetchPromise | ||
| .then(async (response) => { | ||
| if (response.ok) { | ||
| entry.snapshot = await snapshotRscResponse(response); | ||
| } else { | ||
| prefetched.delete(rscUrl); | ||
| cache.delete(rscUrl); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| prefetched.delete(rscUrl); | ||
| cache.delete(rscUrl); | ||
| }) | ||
| .finally(() => { | ||
| entry.pending = undefined; | ||
| }); | ||
|
|
||
| cache.set(rscUrl, entry); | ||
| } |
There was a problem hiding this comment.
prefetchRscResponse() adds entries to the shared prefetch cache without enforcing MAX_PREFETCH_CACHE_SIZE or sweeping expired entries. On link-heavy pages this can cause unbounded growth of both __VINEXT_RSC_PREFETCH_CACHE__ and __VINEXT_RSC_PREFETCHED_URLS__ (successful prefetches are never evicted unless navigated to). Consider reusing the same eviction/TTL sweep logic as storePrefetchResponse() before cache.set() (and deleting from prefetched when evicting).
| export function consumePrefetchResponse(rscUrl: string): CachedRscResponse | null { | ||
| const cache = getPrefetchCache(); | ||
| const entry = cache.get(rscUrl); | ||
| if (!entry) return null; | ||
|
|
||
| // Don't consume pending entries — let the navigation fetch independently. | ||
| if (entry.pending) return null; | ||
|
|
||
| cache.delete(rscUrl); | ||
| getPrefetchedUrls().delete(rscUrl); | ||
|
|
||
| if (entry.snapshot) return entry.snapshot; | ||
|
|
||
| // Legacy: raw Response entries (from storePrefetchResponse) | ||
| // These can't be consumed synchronously as snapshots — skip them. | ||
| // The navigation code will re-fetch. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
consumePrefetchResponse() does not check PREFETCH_CACHE_TTL before returning a cached snapshot. As written, a prefetched RSC payload can be consumed arbitrarily long after it was fetched, which can serve stale UI and defeats the TTL constant/comment above. Suggest: if Date.now() - entry.timestamp >= PREFETCH_CACHE_TTL, delete the entry + prefetched marker and return null.
| function evictVisitedResponseCacheIfNeeded(): void { | ||
| while (visitedResponseCache.size >= MAX_VISITED_RESPONSE_CACHE_SIZE) { | ||
| const oldest = visitedResponseCache.keys().next().value; | ||
| if (oldest === undefined) { | ||
| return; | ||
| } | ||
| visitedResponseCache.delete(oldest); | ||
| } | ||
| } |
There was a problem hiding this comment.
evictVisitedResponseCacheIfNeeded() evicts the oldest inserted entry (Map insertion order), but cache hits in getVisitedResponse() don't refresh recency. This makes the behavior FIFO rather than true LRU as described in the PR summary, and can evict an entry that was just used. If LRU is intended, update the entry's insertion order on read (delete+set) or maintain an explicit recency structure.
…ams eagerly createFromFetch() returns a Thenable (not instanceof Promise), so the async branch was never taken. The raw unresolved Thenable was set as React children, causing suspension without a Suspense boundary and empty content after cross-route navigations. - Duck-type the .then() check to handle both Promises and Thenables - Replace stageClientParams with applyClientParams so useSyncExternalStore subscribers see correct params immediately - Remove dead stageClientParams function and replaceClientParamsWithoutNotify import Fixes useParams E2E failures in CI (hooks.spec.ts:161, hooks.spec.ts:180).
The Flight Thenable from createFromFetch was passed unresolved to renderNavigationPayload/updateBrowserTree. The .then() callback fired unreliably across consecutive navigations, causing empty content. Await createFromFetch in both navigation paths (cached and fresh) so renderNavigationPayload always receives a resolved ReactNode. Add stale navigation checks after the new await points. The HMR handler and server action handler already awaited correctly.
Preserve the X-Vinext-Params header when snapshotting prefetched and visited RSC responses so cached navigations keep dynamic params intact. Stage client params until the URL commit lands so useParams, history state, and redirect handling stay aligned during App Router transitions.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Firefox cross-route navigation hang fix (#652)
This is a substantial and well-thought-out rework of the client navigation system. The core insight — that Firefox's startTransition scheduler stalls when replacing the entire RSC tree — is sound, and the two-phase approach (synchronous for cross-route, startTransition for same-route) is a pragmatic solution. The deferred URL commit via useLayoutEffect, the snapshot context for hook consistency, and the navigation ID counter for stale-navigation bailout are all good architectural choices.
The test coverage is thorough: Playwright E2E tests covering cross-route, same-route, back/forward, rapid clicking, hook sync, and round-trip SPA state.
I have a few concerns, mostly around edge cases in the caching layers and one correctness issue with the navigation flow.
Major concerns
-
Double history push for App Router navigations: In
navigateClientSide()(navigation.ts:906-912), the URL is pushed viapushHistoryStateWithoutNotifybefore calling__VINEXT_RSC_NAVIGATE__. Then insidenavigateRsc(app-browser-entry.ts:514),createNavigationCommitEffectis created with the samehistoryUpdateMode, and the commit effect (line 142-156) also pushes/replaces history ifwindow.location.href !== targetHref. SincenavigateClientSidealready pushed, the location should match and the effect becomes a no-op — but this is fragile. If any middleware or redirect changes the URL between these two points, you could get a double push. Consider either removing the early push innavigateClientSidefor RSC navigations and letting the commit effect handle it exclusively, or documenting why both are needed. -
prefetchRscResponse()does not enforce cache size limits: The Copilot review already flagged this.prefetchRscResponse()callscache.set()without any eviction check. On link-heavy pages, this can cause unbounded growth. The oldstorePrefetchResponse()has proper eviction logic — consider reusing it or adding equivalent bounds toprefetchRscResponse(). -
consumePrefetchResponse()ignores TTL: Also flagged by Copilot. A prefetched snapshot can be consumed well past itsPREFETCH_CACHE_TTL. CheckDate.now() - entry.timestamp >= PREFETCH_CACHE_TTLbefore returning the snapshot.
Minor concerns
-
FIFO cache labeled LRU: The PR description says "visited response cache with LRU eviction" but
getVisitedResponse()does not refresh recency on hit. This is FIFO, which is fine for a 50-entry cache, but the PR description should be updated or the cache should be promoted on access (delete+set to move to end of Map insertion order). -
Traversal bypasses TTL on visited cache:
getVisitedResponse()returns cached entries unconditionally for"traverse"navigations (line 182-183). This is intentional for instant back/forward, but means a user could see arbitrarily stale content after going back. Consider whether there should be an upper bound (e.g., 30 minutes) even for traversals. -
navigationSnapshotActiveCountcan leak:activateNavigationSnapshot()increments the counter, but decrement only happens incommitClientNavigationState(). If a navigation throws before reaching commit (e.g., thecreateFromFetchpromise rejects after theactivateNavigationSnapshotcall inrenderNavigationPayload), the counter increments but never decrements. The catch block at line 604 callscommitClientNavigationState()which decrements, but only if the error is in the outernavigateRsctry block. Errors insiderenderNavigationPayload→updateBrowserTree(async promise chain) could leak. -
BrowserRootsets module-level state in render:setBrowserTreeState = setTreeStateat line 247 runs during every render ofBrowserRoot. This is a side effect during render, which React Strict Mode may call twice. It works in practice because the setter is stable, but it's worth noting this pattern is fragile and could break under concurrent features.
Nits
| entry.pending = undefined; | ||
| }); | ||
|
|
||
| cache.set(rscUrl, entry); |
There was a problem hiding this comment.
prefetchRscResponse() adds entries without enforcing MAX_PREFETCH_CACHE_SIZE. On link-heavy pages (hundreds of Link components), the prefetch cache can grow unbounded. The old storePrefetchResponse() has eviction logic — consider adding the same sweep+FIFO eviction here before cache.set().
Also, getPrefetchedUrls() entries for successfully-prefetched URLs are never evicted unless the URL is navigated to via consumePrefetchResponse(). If the user never visits those routes, they accumulate forever.
| cache.delete(rscUrl); | ||
| getPrefetchedUrls().delete(rscUrl); | ||
|
|
||
| if (entry.snapshot) return entry.snapshot; |
There was a problem hiding this comment.
This returns the snapshot without checking TTL. A prefetched entry that's 5 minutes old will still be consumed here, serving stale UI. Consider:
| if (entry.snapshot) return entry.snapshot; | |
| if (entry.snapshot) { | |
| if (Date.now() - entry.timestamp >= PREFETCH_CACHE_TTL) { | |
| // Expired — discard and let navigation re-fetch | |
| return null; | |
| } | |
| return entry.snapshot; | |
| } |
| } | ||
| visitedResponseCache.delete(oldest); | ||
| } | ||
| } |
There was a problem hiding this comment.
PR summary says "LRU eviction" but this is FIFO — getVisitedResponse() doesn't refresh insertion order on cache hits. For true LRU, add visitedResponseCache.delete(rscUrl) + visitedResponseCache.set(rscUrl, cached) in getVisitedResponse() when returning a hit. Or just update the PR description to say FIFO — it's perfectly fine for a 50-entry cache.
| if (typeof window.__VINEXT_RSC_NAVIGATE__ === "function") { | ||
| await window.__VINEXT_RSC_NAVIGATE__(fullHref); | ||
| if (mode === "replace") { |
There was a problem hiding this comment.
For App Router RSC navigations, this pushes the URL into history before calling __VINEXT_RSC_NAVIGATE__, and then createNavigationCommitEffect inside navigateRsc will also attempt to push/replace history (guarded by window.location.href !== targetHref). The guard makes the second push a no-op in the normal case, but the intent would be clearer if navigateClientSide skipped the early push for RSC navigations and let the commit effect own it exclusively. The current code works but is fragile — a redirect could cause the URLs to diverge between the two push sites.
| navigationSnapshot: initialNavigationSnapshot, | ||
| }); | ||
|
|
||
| setBrowserTreeState = setTreeState; |
There was a problem hiding this comment.
Setting module-level state (setBrowserTreeState) during render is a side effect that React Strict Mode or concurrent rendering could call multiple times. This works because setTreeState is referentially stable, but it would be more robust to use a useEffect or useRef pattern:
| setBrowserTreeState = setTreeState; | |
| const setterRef = useRef(setTreeState); | |
| setterRef.current = setTreeState; | |
| useEffect(() => { | |
| setBrowserTreeState = setterRef.current; | |
| }); |
Not blocking, but worth considering for future React versions.
| pendingNavigationCommits.set(renderId, resolve); | ||
| }); | ||
|
|
||
| activateNavigationSnapshot(); |
There was a problem hiding this comment.
If the promise chain in updateBrowserTree rejects (e.g., createFromFetch fails after the snapshot is activated), activateNavigationSnapshot() has already incremented the counter but commitClientNavigationState() may never be called for this navigation, causing navigationSnapshotActiveCount to leak. The hooks would then permanently prefer stale snapshot values over useSyncExternalStore.
Consider wrapping renderNavigationPayload with a .catch() that calls commitClientNavigationState() to ensure the counter is always balanced.
| return null; | ||
| } | ||
|
|
||
| if (navigationKind === "traverse") { |
There was a problem hiding this comment.
Traversal ("traverse") returns cached entries regardless of expiresAt. This means back/forward can show arbitrarily old content if the user waits long enough. This is probably the right tradeoff for instant back/forward UX, but consider whether a generous upper bound (e.g., 30 minutes) would be appropriate to avoid very stale pages.
| * mounts, so its pre-paint effect never fires. By draining all effects | ||
| * <= the committed renderId here, the winning transition cleans up after | ||
| * any superseded ones, keeping the counter balanced. | ||
| */ | ||
| function drainPrePaintEffects(upToRenderId: number): void { |
There was a problem hiding this comment.
Good design: draining all effects ≤ the committed renderId handles the superseded-transition cleanup. One subtlety: for superseded renderIds (id < upToRenderId), you call commitClientNavigationState() which decrements navigationSnapshotActiveCount. This is correct since each was activated by renderNavigationPayload. Worth a brief inline comment noting that the 1:1 activate/commit pairing is maintained here.
| */ | ||
| export function activateNavigationSnapshot(): void { | ||
| const state = getClientNavigationState(); | ||
| if (state) state.navigationSnapshotActiveCount++; |
There was a problem hiding this comment.
This only increments; decrement happens in commitClientNavigationState(). The 1:1 pairing relies on drainPrePaintEffects cleaning up superseded navigations, and the catch block in navigateRsc calling commitClientNavigationState(). If you add error handling in renderNavigationPayload per my other comment, this becomes more robust. Consider adding a doc comment noting the pairing invariant.
| } else { | ||
| pushHistoryStateWithoutNotify(null, "", fullHref); | ||
| } | ||
| await window.__VINEXT_RSC_NAVIGATE__(fullHref, 0, "navigate", mode); |
There was a problem hiding this comment.
Nit: the historyUpdateMode parameter is only meaningful for the commit effect inside navigateRsc. Since navigateClientSide already pushed/replaced the URL at lines 907-911 (before calling this), passing mode here is only needed so the commit effect knows to push again if the URL was changed by a redirect. This dual-purpose is confusing — a comment explaining why both the caller and callee touch history would help future readers.
|
The review has been submitted successfully. Here's a summary of the feedback I posted: Major concerns (3):
Minor concerns (4): Inline comments (10) with specific code suggestions on the affected lines. |
…ment - Remove double history push for RSC navigations: let the commit effect in navigateRsc own push/replace exclusively. Also fixes a hidden bug where the early push broke isSameRoute detection (always true), negating the Firefox cross-route hang fix. - Enforce prefetch cache size limits in prefetchRscResponse() by extracting shared eviction logic from storePrefetchResponse(). - Add TTL check in consumePrefetchResponse() so expired snapshots are discarded instead of served as stale UI. - Prevent navigationSnapshotActiveCount leak when async payloads reject in updateBrowserTree by adding error handlers that balance the activate/commit pairing. - Move BrowserRoot setBrowserTreeState assignment from render to useLayoutEffect to avoid side effects under React Strict Mode. - Make visited response cache true LRU (delete+set on hit) and add a 30-minute upper bound for traversal cache hits to prevent arbitrarily stale back/forward content.
|
This is the same PR as mine... #643 |
ah, didn't realize it. I'll close this one |
Summary
Fixes #652 —
startTransitionnever commits in Firefox when the entire RSC component tree is replaced during cross-route navigation. The old page stays visible indefinitely in production builds on workerd.Root cause: vinext replaces the full RSC tree on every cross-route navigation inside
startTransition. The React transition scheduler cannot finalize this in Firefox. The previousflushSyncapproach also caused the Suspense double-flash from #639.Fix: Two-phase navigation commit with same-route detection:
startTransitionfor smooth incremental updatesuseLayoutEffectso hooks see the pending URL during transitionsKey changes
navigation.ts:ClientNavigationStateonSymbol.forglobal (survives multiple module instances), render snapshot context for hook consistency during transitions, RSC response snapshot/restore for visited cache, unifiednavigateClientSide()entry point, history suppression helpersapp-browser-entry.ts:BrowserRootcomponent withuseState-managed tree,NavigationCommitSignalthat defers URL commit touseLayoutEffect, visited response cache with LRU eviction (50 entries, 5min TTL), navigation ID counter for stale navigation bailout, manual scroll restorationlink.tsx: removed duplicatedisHashOnlyChange/scrollToHash, delegates tonavigateClientSideform.tsx: App Router GET forms delegate tonavigateClientSideglobal.d.ts: extended__VINEXT_RSC_NAVIGATE__signature withnavigationKindandhistoryUpdateModeEdge cases handled
location.assign)Symbol.forglobal state)Test plan
vp checkpasses (only pre-existing benchmark errors remain)tests/shims.test.ts— 742 tests passtests/form.test.ts— updated assertions for new navigate signature, all passtests/prefetch-cache.test.ts— compatible with newPrefetchCacheEntrytype, all passtests/link.test.ts— all passtests/routing.test.ts— all passtests/app-router.test.ts— all passtests/entry-templates.test.ts— all passtests/e2e/app-router/navigation-regressions.spec.tscovering:usePathname/useSearchParams/useParamshook sync